fix(sqlite-store): reject zero partial_blockchain_nodes.id instead of panicking - #2462
fix(sqlite-store): reject zero partial_blockchain_nodes.id instead of panicking#2462ygd58 wants to merge 2 commits into
Conversation
… panicking parse_partial_blockchain_nodes called NonZeroUsize::new(id).unwrap() on a u64 id column read directly from the local sqlite database. InOrderIndex::new requires a NonZeroUsize (from_leaf_pos's minimum output is 1 - pos * 2 - 1 with pos >= 1), so a stored id of 0 is never something the client's own write path produces - it can only come from a corrupted or externally-tampered-with local database. Routes this through the function's existing Result<_, StoreError> instead, using the existing StoreError::ParsingError variant (no new variant needed) - the same pattern already used for the sibling id/usize try_from and the node Word::read_from_bytes conversions two lines below, which already return errors rather than panicking. Adds a direct unit test constructing a zero-id row and asserting the error, without needing the full store/connection test harness.
This is an edge case too far-fetched to consider handling by surfacing an error, the current |
|
Fair point, updated - kept it a panic, just with a message that names the actual problem ( |
|
Verified the updated version locally too: New test: |
2 participants
Searched for existing issues/PRs (
NonZeroUsize,InOrderIndex,partial_blockchain_nodes,parse_partial_blockchain_nodes) before writing this - #1691 is the only related hit and it's a different concern (32-bitusizelimiting the max representable index in WASM builds, not this panic).Bug
parse_partial_blockchain_nodescalledNonZeroUsize::new(id).unwrap()on au64idcolumn read directly from the local sqlite database.InOrderIndex::newrequires aNonZeroUsize(from_leaf_pos's minimum output is 1 -pos * 2 - 1withpos >= 1), so a storedidof0is never something the client's own write path produces - it can only come from a corrupted or externally-tampered-with local database, and would panic the client instead of surfacing an error.Fix
Routes this through the function's existing
Result<_, StoreError>instead, using the already-existingStoreError::ParsingErrorvariant - no new error variant needed, and matches the pattern the two adjacent conversions in the same function already use (theid/usize::try_fromand thenode/Word::read_from_bytesboth already return errors rather than panicking; this brings the third field in line).Test plan
Added a direct unit test constructing a zero-id row and asserting the error, without needing the full store/connection test harness. Ran locally, not just CI:
1 passed. Also ran the full crate suite:91 passed, 0 failed- no regressions.